Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify Intel RDT configuration #1196

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ipuustin
Copy link

@ipuustin ipuustin commented Apr 6, 2023

First change: make it explicit when l3CacheSchema should have lines with MB: prefix filtered out. Previously it was unclear if this should be done always or only if memBwSchema did exist.

Second change: clarify when the subdirectory needs to be removed after the container is finished (not sure if the config spec is the best place for this?).

@utam0k
Copy link
Member

utam0k commented Apr 6, 2023

Does this change require a change in runc behavior?

@ipuustin
Copy link
Author

ipuustin commented Apr 6, 2023

Yes it does... I don't think runc filters the MB:-lines at all -- it just concatenates the values. Also for directory removal, runc removes the directory if and only if ClosID is not set. It doesn't take into account whether or not runc has created the directory to begin with.

@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Apr 12, 2023
@utam0k
Copy link
Member

utam0k commented Apr 12, 2023

I have looked at the major container runtime implementations, and only runc is affected. It would be good to have @AkihiroSuda, @kolyshkin, and @cyphar review this.

@giuseppe
Just to be sure, crun doesn't implement Intel RDT, does it?

@giuseppe
Copy link
Member

@giuseppe
Just to be sure, crun doesn't implement Intel RDT, does it?

correct, it is not implemented in crun

@utam0k
Copy link
Member

utam0k commented Apr 13, 2023

@giuseppe Thanks for your quick response 🙏

@ipuustin
Copy link
Author

I have looked at the major container runtime implementations, and only runc is affected. It would be good to have @AkihiroSuda, @kolyshkin, and @cyphar review this.

@giuseppe Just to be sure, crun doesn't implement Intel RDT, does it?

Thanks for looking at this! I implemented the required runc changes now (in a linked PR).

@AkihiroSuda AkihiroSuda modified the milestones: v1.2.0, v1.1.1 Apr 20, 2023
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kolyshkin
Copy link
Contributor

First change: make it explicit when l3CacheSchema should have lines with MB: prefix filtered out. Previously it was unclear if this should be done always or only if memBwSchema did exist.

This is kind of weird. Runtime is not a tool to groom configs. Why don't we just require that if both l3CacheSchema and memBwSchema exist, the first one should not contain lines with MB: prefix? This would be way easier to implement in config validation (strings.HasPrefix(s, "MB:") || strings.Contains(s, "\nMB:")) than the filtering.

@kolyshkin
Copy link
Contributor

Alternatively, if the kernel is OK with getting multiple MB: strings, we can just write both l3CacheSchema and memBwSchema and the latter will naturally overwrite the former.

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have a question about the requirement of filtering out MB: entries from l3CacheSchema.

@ipuustin
Copy link
Author

TBH I'm not sure why this is written the way it is now, but I think this may be due to originally L3 cache being the only supported resource (see opencontainers/runc#433). The reason for l3CacheSchema containing MB: strings is probably for backwards compatibility during the transitioning from l3CacheSchema to having both l3CacheSchema and memBwSchema.

My target with this PR was to clarify the spec from the implementation point of view, but I fully agree that removing the filtering would make things simpler. I would however lean more towards your first option (erroring out if there are conflicting values). Having the "silent override" would probably confuse users and cause hard-to-debug bugs.

Erroring out in case of conflict would be a breaking change though -- I don't know if there are any workloads which currently set MB: strings from both variables. @marquiz since the previous wording is from your PR, do you have any insight or opinion into this?

@marquiz
Copy link
Contributor

marquiz commented May 24, 2023

Alternatively, if the kernel is OK with getting multiple MB: strings, we can just write both l3CacheSchema and memBwSchema and the latter will naturally overwrite the former.

I think this should actually be true

The thinking is that the runtimes should not do the filtering of values,
but instead just apply the values in order. This way the possible
MB-lines in l3CacheSchema will become overwritten by memBwSchema values
(if the domains overlap).

Note that we can't just concatenate the values because kernel will error
out if the same domain is attempted to be set multiple times within one
write() call.

Signed-off-by: Ismo Puustinen <[email protected]>
@ipuustin
Copy link
Author

ipuustin commented Jun 2, 2023

@kolyshkin @marquiz I now updated the PR to say that the schemata changes should be applied in order. This is in line with the -"natural override" proposal and also maybe what the user has in mind. This will be a breaking change however, because now we don't filter anything out. If the MB domains set in l3CacheSchema are different than the domains in memBwSchema, both will be set.

@marquiz
Copy link
Contributor

marquiz commented Sep 28, 2023

I now updated the PR to say that the schemata changes should be applied in order. This is in line with the -"natural override" proposal and also maybe what the user has in mind. This will be a breaking change however, because now we don't filter anything out. If the MB domains set in l3CacheSchema are different than the domains in memBwSchema, both will be set.

@ipuustin looking at this now, I'm inclined against a breaking change in the spec. Even though I don't think any runtime actually implements the filtering of data. But LGTM for the additions about directory removal

Related: with a "leave the old mess behind" mentality, I submitted PR #1230.

@AkihiroSuda
Copy link
Member

@kolyshkin PTAL

@AkihiroSuda AkihiroSuda modified the milestones: v1.1.1, v1.1.2 Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants